use ET timezone consistently in the admin UI #693
Conversation
GUI
left a comment
GUI
left a comment
There was a problem hiding this comment.
@jtmkrueger, looks great overall! Just had one proposed tweak to the front-end code to respect a system-wide setting that can control the desired timezone. I'm hoping that session data should be available in this context, but holler if you run into any issues with that.
| renderTime(value, type) { | ||
| if(type === 'display' && value && value !== '-') { | ||
| return moment(value).format('YYYY-MM-DD HH:mm:ss'); | ||
| return moment(value).tz('America/New_York').format('YYYY-MM-DD HH:mm:ss z'); |
There was a problem hiding this comment.
There's a system-wide analytics.timezone configuration setting that is used throughout the system to determine this type of default time zone to be used. This system setting is exposed during the login in some session data, and that's how the charts know what time zone to use:
So to remain consistent with this configuration setting, I'd maybe recommend using that same setting here instead of hard-coding it to ET (even though for the api.data.gov deploy, it's hard-coded to ET):
| return moment(value).tz('America/New_York').format('YYYY-MM-DD HH:mm:ss z'); | |
| return moment(value).tz(this.session.data.authenticated.analytics_timezone).format('YYYY-MM-DD HH:mm:ss z'); |
There was a problem hiding this comment.
Oh I didn't spot this, cool! Changed to utilize. Since there's no this.session context here I had to refactor some things, and change how renderTime is structured. Used the same pattern implemented in other functions like renderLinkedList here in data-tables-helpers.js, but then had to adapt call locations to use the new method signature. I didn't go with a plain options but just set things up to explicitly require passing in this.session.data.authenticated.analytics_timezone when calling renderTime. This required touching a number of other files including injecting session into relevant results-table.js's. If you think there's a cleaner way let me know!
test/admin_ui/test_stats_logs.rb
Outdated
| def test_table_displays_time_in_eastern_timezone | ||
| FactoryBot.create(:log_item, :request_at => Time.parse("2015-01-16T06:06:28.816Z").utc, :request_method => "OPTIONS") | ||
| LogItem.refresh_indices! | ||
|
|
||
| admin_login | ||
| visit "/admin/#/stats/logs?search=&start_at=2015-01-12&end_at=2015-01-18&interval=day" | ||
| refute_selector(".busy-blocker") | ||
|
|
||
| assert_text("2015-01-16 01:06:28 EST") | ||
| end | ||
|
|
||
| def test_table_displays_time_in_eastern_timezone_during_dst | ||
| FactoryBot.create(:log_item, :request_at => Time.parse("2015-07-16T06:06:28.816Z").utc, :request_method => "OPTIONS") | ||
| LogItem.refresh_indices! | ||
|
|
||
| admin_login | ||
| visit "/admin/#/stats/logs?search=&start_at=2015-07-12&end_at=2015-07-18&interval=day" | ||
| refute_selector(".busy-blocker") | ||
|
|
||
| assert_text("2015-07-16 02:06:28 EDT") | ||
| end |
There was a problem hiding this comment.
Thanks for adding this test coverage! Related to my last comment about leveraging the analytics.timezone setting, the test environment is hard-coded to use America/Denver:
Lines 6 to 7 in 1cb2bd5
So if you start leveraging that setting, I think you'll need to update these tests to use MDT/MST for their frame of reference. There's technically ways you could also change the timezone in specific tests, but I think that's probably overkill here, so I think just updating these tests to verify the mountain time assumption is sufficient.
There was a problem hiding this comment.
Yep, I updated the tests to use MDT
| }, data) | ||
| end | ||
|
|
||
| def test_csv_timestamps_in_iso8601_utc_format |
There was a problem hiding this comment.
This test looks good, but there's one other semi-related test that's failing now: https://github.com/NatLabRockies/api-umbrella/actions/runs/21634777879/job/62359925312#step:5:385
api-umbrella/test/apis/admin/stats/test_users.rb
Lines 99 to 112 in 1cb2bd5
I think you'll need to update those tests to call user1.created_at.utc.iso8601 and then updated the hard-coded "2015-01-16 00:00:00" strings into ISO8601 format.
There was a problem hiding this comment.
Ah, thank you! I was trying to figure out what I was missing on these failures. Fixed!
2 participants
Resolves 18F/api.data.gov#695
The code here utilizes the pre-existing
format_iso8601_msand removes theformat_csvformat. I suppose that a compromise could be made to create a new format that is more like the csv format being depricated, but I figured it'd be nicer to just have less formats to worry about and reuse one we're already using.In the JS, just added some specifics to Parse the timezone as ET and add the timezone to the format string that's being shown to users.
Also adds a couple new tests for these changes.